Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allows check-ins of unreassignable licenses #16063

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

Godmartinz
Copy link
Collaborator

@Godmartinz Godmartinz commented Jan 13, 2025

This adds the ability to check in unreassignable licenses. After checking in an unreassignable license the seat will be disabled for checkout and marked in the database so no API checkouts can occur.

This also gives the ability to bulk check in an unreassignable license with the same outcomes.

image The available seat count is also updated accordingly on the seats tab, the index page, and the licenses reports. image image API attempt to checkout an unreassignable: image

[sc-14999]

Copy link

what-the-diff bot commented Jan 13, 2025

PR Summary

  • Introduction of New Model References
    Imported Actionlog and LicenseSeat models into the Helper.php file to allow for advanced functionality.

  • Addition of New Method
    Created a new method unReassignableCount($license) in Helper.php that tallies up the number of licenses that can't be reassigned.

  • Revised LicenseCheckinController:

    • Altered the license check-in control, removing the need to check if a license is reassignable before checking-in.
    • Enhanced the note-keeping for licenses that can't be reassigned, allowing for more accurate record-keeping during the check-in process.
    • Changed how events are initiated to consider the revised notes.
  • Upgraded LicensesController:

    • Refactored computation of checkedout_seats_count to consider licenses that can't be reassigned.
    • Following this, unreassignable_seats_count is sent for viewing.
  • Transformation of LicenseSeatsTransformer:

    • Incorporated a new method unReassignable($seat) to verify if a seat is non-reassignable.
    • Altered the transformation output to feature a 'disabled' field.
  • Modification of LicensesTransformer:

    • Tweaked the computation of free_seats_count to deduct the count of unReassignable licenses.
  • Revision of License Model:

    • Altered remaincount() to subtract unreassignable licenses, providing a more accurate count of available licenses.
  • Upgraded Blade Templates:

    • Optimized the licenses' view to display available license count subject to unreassignable seats.
    • Updated the interaction with the check-in button when a seat is disabled as depicted in bootstrap-table.
  • Language Files Update:

    • Enhanced clarity by updating the message for non-reassignable licenses to 'Seat has been used' instead.

@Godmartinz
Copy link
Collaborator Author

disabled button color has been fixed 👍

@Godmartinz
Copy link
Collaborator Author

looking into failing tests

@Godmartinz Godmartinz marked this pull request as draft January 14, 2025 20:24
@Godmartinz Godmartinz marked this pull request as ready for review January 16, 2025 19:53
@snipe
Copy link
Owner

snipe commented Jan 16, 2025

Not sure I love the field dead for this.

@snipe
Copy link
Owner

snipe commented Jan 16, 2025

(Also, tests are failing)

@marcusmoore
Copy link
Collaborator

Not sure I love the field dead for this.

😆 @Godmartinz and I were going back and forth on how best to communicate what the column represents.

@snipe
Copy link
Owner

snipe commented Jan 16, 2025

@marcusmoore - unassignable would probably work fine, but I'd probably return a true or false if it's meant to be boolean. Or reverse the logic and use reassignable.

@marcusmoore
Copy link
Collaborator

@snipe we (me really) were trying to avoid confusion around a license seat having assigned_to and unassignable (or a similar word) both be populated at the same time. That really threw me off and I wanted to find another word to signify "this seat might be checked out now but it cannot be checked out again in the future".

Maybe other devs won't find that as confusing as I did.

@snipe
Copy link
Owner

snipe commented Jan 16, 2025

@marcusmoore reassignable_seat perhaps?

@marcusmoore
Copy link
Collaborator

marcusmoore commented Jan 16, 2025

@snipe that would be a lot clearer for me. Though I would go with reassignable so we don't have a redundant word in there $licenseSeat-> reassignable_seat // @Godmartinz

@Godmartinz Godmartinz requested a review from snipe January 16, 2025 20:29
Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bones are solid but I think there is some improvements that can be made.

app/Helpers/Helper.php Outdated Show resolved Hide resolved
app/Helpers/Helper.php Outdated Show resolved Hide resolved
app/Models/License.php Outdated Show resolved Hide resolved
app/Http/Controllers/Licenses/LicenseCheckinController.php Outdated Show resolved Hide resolved
app/Http/Controllers/Licenses/LicensesController.php Outdated Show resolved Hide resolved
app/Http/Transformers/LicenseSeatsTransformer.php Outdated Show resolved Hide resolved
app/Http/Transformers/LicenseSeatsTransformer.php Outdated Show resolved Hide resolved
app/Http/Transformers/LicensesTransformer.php Show resolved Hide resolved
Godmartinz and others added 2 commits January 21, 2025 11:43
Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple minor things (testing)

app/Http/Controllers/Api/LicenseSeatsController.php Outdated Show resolved Hide resolved
tests/Feature/Checkins/Ui/LicenseCheckinTest.php Outdated Show resolved Hide resolved
app/Http/Controllers/Licenses/LicenseCheckinController.php Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants